refactor(depinject/appconfig): remove api module dependency #20931
Conversation
WalkthroughWalkthroughThe changes primarily focus on enhancing the Changes
Sequence Diagram(s)No sequence diagrams are needed as the changes are mostly related to internal structuring, dependency updates, and configuration adjustments without introducing significant new features or control flow modifications. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
aaronc
changed the title
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
aaronc
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files ignored due to path filters (6)
depinject/appconfig/v1alpha1/config.pb.gois excluded by!**/*.pb.godepinject/appconfig/v1alpha1/module.pb.gois excluded by!**/*.pb.godepinject/appconfig/v1alpha1/query.pb.gois excluded by!**/*.pb.godepinject/go.sumis excluded by!**/*.sumdepinject/internal/appconfiggogo/testpb/test.pb.gois excluded by!**/*.pb.gox/staking/types/staking.pb.gois excluded by!**/*.pb.go
Files selected for processing (15)
- api/cosmos/staking/v1beta1/staking.pulsar.go (4 hunks)
- depinject/appconfig/config.go (5 hunks)
- depinject/go.mod (2 hunks)
- depinject/internal/appconfig/buf.gen.pulsar.yaml (1 hunks)
- depinject/internal/appconfig/buf.yaml (1 hunks)
- depinject/internal/appconfig/registry.go (3 hunks)
- depinject/internal/appconfig/testpb/test.proto (1 hunks)
- depinject/internal/appconfig/testpb/test.pulsar.go (2 hunks)
- depinject/internal/appconfiggogo/buf.gen.gogo.yaml (1 hunks)
- depinject/internal/appconfiggogo/buf.yaml (1 hunks)
- depinject/internal/appconfiggogo/testpb/test.proto (1 hunks)
- proto/cosmos/app/v1alpha1/config.proto (1 hunks)
- proto/cosmos/app/v1alpha1/module.proto (1 hunks)
- proto/cosmos/app/v1alpha1/query.proto (1 hunks)
- x/staking/proto/cosmos/staking/v1beta1/staking.proto (2 hunks)
Files not summarized due to errors (1)
- api/cosmos/staking/v1beta1/staking.pulsar.go: Error: Message exceeds token limit
Files skipped from review due to trivial changes (7)
- depinject/internal/appconfig/buf.gen.pulsar.yaml
- depinject/internal/appconfig/buf.yaml
- depinject/internal/appconfig/testpb/test.proto
- depinject/internal/appconfiggogo/buf.gen.gogo.yaml
- depinject/internal/appconfiggogo/buf.yaml
- depinject/internal/appconfiggogo/testpb/test.proto
- x/staking/proto/cosmos/staking/v1beta1/staking.proto
Additional context used
Path-based instructions (4)
depinject/internal/appconfig/registry.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.depinject/appconfig/config.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.depinject/internal/appconfig/testpb/test.pulsar.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.api/cosmos/staking/v1beta1/staking.pulsar.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
GitHub Check: break-check
proto/cosmos/app/v1alpha1/query.proto
[failure] 7-7:
File option "go_package" changed from "cosmossdk.io/api/app/v1alpha1" to "cosmossdk.io/depinject/appconfig/v1alpha1".proto/cosmos/app/v1alpha1/config.proto
[failure] 7-7:
File option "go_package" changed from "cosmossdk.io/api/app/v1alpha1" to "cosmossdk.io/depinject/appconfig/v1alpha1".proto/cosmos/app/v1alpha1/module.proto
[failure] 7-7:
File option "go_package" changed from "cosmossdk.io/api/app/v1alpha1" to "cosmossdk.io/depinject/appconfig/v1alpha1".
Additional comments not posted (14)
proto/cosmos/app/v1alpha1/query.proto (1)
7-7: Update go_package to reflect new package path.The
go_packageoption has been correctly updated to match the new package organization. This is consistent with the changes described in the PR summary.Tools
GitHub Check: break-check
[failure] 7-7:
File option "go_package" changed from "cosmossdk.io/api/app/v1alpha1" to "cosmossdk.io/depinject/appconfig/v1alpha1".depinject/go.mod (1)
10-10: Update dependencies to reflect package reorganization.The dependency on
google.golang.org/grpchas been removed, andgithub.com/tendermint/go-aminohas been added as an indirect dependency. These changes are consistent with the package reorganization described in the PR summary.Also applies to: 22-22
proto/cosmos/app/v1alpha1/config.proto (1)
7-7: Update go_package to reflect new package path.The
go_packageoption has been correctly updated to match the new package organization. This is consistent with the changes described in the PR summary.Tools
GitHub Check: break-check
[failure] 7-7:
File option "go_package" changed from "cosmossdk.io/api/app/v1alpha1" to "cosmossdk.io/depinject/appconfig/v1alpha1".depinject/internal/appconfig/registry.go (3)
8-9: Add necessary imports for protobuf handling.The new imports from
google.golang.org/protobufandgithub.com/cosmos/gogoproto/protoc-gen-gogo/descriptorare appropriate and necessary for handling protobuf descriptors.Also applies to: 11-11
41-41: Enhance module descriptor handling in ModulesByModuleTypeName.The changes to the
ModulesByModuleTypeNamefunction improve the handling and error reporting of module descriptors using the newGetModuleDescriptorfunction.Also applies to: 44-46, 52-56
74-116: Add GetModuleDescriptor function for module descriptor handling.The new
GetModuleDescriptorfunction effectively handles module descriptors with comprehensive error handling and compatibility with both gogoproto and protoreflect types.proto/cosmos/app/v1alpha1/module.proto (1)
7-7: Approve the change forgo_packageoption.The
go_packageoption has been updated to reflect the new package path, ensuring consistency across the codebase.Tools
GitHub Check: break-check
[failure] 7-7:
File option "go_package" changed from "cosmossdk.io/api/app/v1alpha1" to "cosmossdk.io/depinject/appconfig/v1alpha1".depinject/appconfig/config.go (2)
24-34: Approve changes inLoadJSON.The function has been updated to use
protojsonanddynamicpb, avoiding direct dependencies on API types. This improves flexibility and maintainability.However, ensure that
gogoproto.HybridResolveranddynamicpb.NewMessageare used correctly and that error handling is appropriate.Verification successful
Verification successful.
The usage of
gogoproto.HybridResolveranddynamicpb.NewMessageis consistent and valid across the codebase. The changes indepinject/appconfig/config.goare appropriate and maintain the expected functionality.
- depinject/appconfig/config.go
- Line 24:
resolver := gogoproto.HybridResolver- Line 30:
config := dynamicpb.NewMessage(desc.(protoreflect.MessageDescriptor))Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `gogoproto.HybridResolver` and `dynamicpb.NewMessage`. # Test: Search for the usage of `gogoproto.HybridResolver`. Expect: Only valid usages. rg --type go 'gogoproto.HybridResolver' # Test: Search for the usage of `dynamicpb.NewMessage`. Expect: Only valid usages. rg --type go 'dynamicpb.NewMessage'Length of output: 3190
Line range hint
64-157: Approve changes inCompose.The function has been updated to accept
gogoproto.Messageinstead ofappv1alpha1.Config, improving flexibility. The conversion logic appears to be correct, and error handling is appropriate.However, ensure that the conversion logic handles all possible cases and that error handling is thorough.
depinject/internal/appconfig/testpb/test.pulsar.go (1)
5-5: Verify the correctness of the new import path.The import path has been updated from
cosmossdk.io/api/cosmos/app/v1alpha1tocosmossdk.io/depinject/appconfig/v1alpha1. Ensure that this new path exists and is correct.Verification successful
Verified: The new import path
cosmossdk.io/depinject/appconfig/v1alpha1is correct and exists in the codebase.The import statement has been successfully updated to align with the refactoring objective and does not introduce any issues.
depinject/internal/appconfig/registry.godepinject/internal/appconfig/testpb/test.pulsar.godepinject/internal/appconfiggogo/testpb/test.pb.godepinject/appconfig/config.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence and correctness of the new import path. # Test: Check if the new import path exists. Expect: The path should exist and be correct. rg --type go 'cosmossdk.io/depinject/appconfig/v1alpha1'Length of output: 413
api/cosmos/staking/v1beta1/staking.pulsar.go (4)
15011-15013: Deprecation ofHistoricalEntriesfield is approved.The
HistoricalEntriesfield is now marked as deprecated. Consider removing this field in a future release to clean up the codebase.
15064-15067: Addition and deprecation ofGetHistoricalEntriesmethod is approved.The function
GetHistoricalEntriesis added and marked as deprecated. Consider removing this method in a future release to clean up the codebase.
15704-15707: Verify consistency of protobuf changes.Ensure that the protobuf changes in this segment are consistent with the rest of the protobuf definitions in the project.
15714-15850: Verify consistency of protobuf changes and encoded data.Ensure that the protobuf changes and encoded data in this segment are consistent with the rest of the protobuf definitions in the project.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files ignored due to path filters (2)
orm/go.sumis excluded by!**/*.sumruntime/v2/go.sumis excluded by!**/*.sum
Files selected for processing (2)
- orm/go.mod (1 hunks)
- runtime/v2/go.mod (1 hunks)
Files skipped from review due to trivial changes (2)
- orm/go.mod
- runtime/v2/go.mod
Co-authored-by: Marko <marko@baricevic.me>
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- depinject/internal/appconfig/registry.go (3 hunks)
Additional context used
Path-based instructions (1)
depinject/internal/appconfig/registry.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
GitHub Check: tests (03)
depinject/internal/appconfig/registry.go
[failure] 41-41:
dp declared and not used
GitHub Check: tests (02)
depinject/internal/appconfig/registry.go
[failure] 41-41:
dp declared and not used
GitHub Check: tests (01)
depinject/internal/appconfig/registry.go
[failure] 41-41:
dp declared and not used
GitHub Check: tests (00)
depinject/internal/appconfig/registry.go
[failure] 41-41:
dp declared and not used
GitHub Check: dependency-review
depinject/internal/appconfig/registry.go
[failure] 41-41:
dp declared and not used
Additional comments not posted (2)
depinject/internal/appconfig/registry.go (2)
73-115: Robust error handling inGetModuleDescriptor.The function has comprehensive error handling for its operations, which is crucial given the complexity of the operations involved. Continue to monitor and test these paths to ensure reliability.
41-52: Ensure robust error handling and logic in module descriptor retrieval.The function correctly handles potential errors when fetching module descriptors. However, ensure that all possible error scenarios are covered, especially in dynamic type conversions and protobuf operations.
Verification successful
Error handling in
ModulesByModuleTypeNamefunction is adequate.The function
ModulesByModuleTypeNameappropriately handles errors through multiple checks and returns, ensuring robust error handling and logic in module descriptor retrieval.
if err != nilchecks are used to handle errors.fmt.Errorfis used to provide descriptive error messages.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify error handling paths in `ModulesByModuleTypeName`. # Test: Search for error handling in the function. Expect: Proper error handling for all operations. rg --type go -A 5 $'error handling in ModulesByModuleTypeName' depinject/internal/appconfig/registry.goLength of output: 104
Script:
#!/bin/bash # Description: Verify error handling within the `ModulesByModuleTypeName` function. # Step 1: Search for the `ModulesByModuleTypeName` function definition. rg 'func ModulesByModuleTypeName' -A 20 depinject/internal/appconfig/registry.go # Step 2: Search for error handling within the `ModulesByModuleTypeName` function. rg 'ModulesByModuleTypeName' -A 50 depinject/internal/appconfig/registry.go | rg 'if err != nil|return nil, err|fmt.Errorf'Length of output: 1167
Tools
GitHub Check: tests (03)
[failure] 41-41:
dp declared and not usedGitHub Check: tests (02)
[failure] 41-41:
dp declared and not usedGitHub Check: tests (01)
[failure] 41-41:
dp declared and not usedGitHub Check: tests (00)
[failure] 41-41:
dp declared and not usedGitHub Check: dependency-review
[failure] 41-41:
dp declared and not used
| @@ -35,12 +38,18 @@ | |||
| fullName := gogoproto.MessageName(initializer.ConfigProtoMessage) | |||
|
|
|||
| if desc, err := gogoproto.HybridResolver.FindDescriptorByName(protoreflect.FullName(fullName)); err == nil { | |||
| modDesc := protov2.GetExtension(desc.Options(), appv1alpha1.E_Module).(*appv1alpha1.ModuleDescriptor) | |||
| dp := protodesc.ToDescriptorProto(desc.(protoreflect.MessageDescriptor)) | |||
There was a problem hiding this comment.
Remove unused variable dp.
The variable dp is declared but not used, which could lead to confusion and clutter in the code.
- dp := protodesc.ToDescriptorProto(desc.(protoreflect.MessageDescriptor))Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| dp := protodesc.ToDescriptorProto(desc.(protoreflect.MessageDescriptor)) |
Tools
GitHub Check: tests (03)
[failure] 41-41:
dp declared and not used
GitHub Check: tests (02)
[failure] 41-41:
dp declared and not used
GitHub Check: tests (01)
[failure] 41-41:
dp declared and not used
GitHub Check: tests (00)
[failure] 41-41:
dp declared and not used
GitHub Check: dependency-review
[failure] 41-41:
dp declared and not used
| // GetModuleDescriptor returns the cosmos.app.v1alpha1.ModuleDescriptor or nil if one isn't found. | ||
| // Errors are returned in unexpected cases. | ||
| func GetModuleDescriptor(desc protoreflect.Descriptor) (*v1alpha1.ModuleDescriptor, error) { | ||
| // we need to take a somewhat round about way to get the extension here | ||
| // our most complete type registry has a mix of gogoproto and protoreflect types | ||
| // so we start with a protoreflect descriptor, convert it to a gogo descriptor | ||
| // and then get the extension by its raw field value to avoid any unmarshaling errors | ||
|
|
||
| rawV2Desc := protodesc.ToDescriptorProto(desc.(protoreflect.MessageDescriptor)) | ||
| bz, err := protov2.Marshal(rawV2Desc) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| var gogoDesc descriptor.DescriptorProto | ||
| err = gogoproto.Unmarshal(bz, &gogoDesc) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| opts := gogoDesc.Options | ||
| if !gogoproto.HasExtension(opts, v1alpha1.E_Module) { | ||
| return nil, nil | ||
| } | ||
|
|
||
| bz, err = gogoproto.GetRawExtension(gogoproto.GetUnsafeExtensionsMap(opts), v1alpha1.E_Module.Field) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| // we have to skip the field tag and length prefix itself to actually get the raw bytes we want | ||
| // this is really overly complex, but other methods caused runtime errors because of validation | ||
| // that gogo does that appears simply not necessary | ||
| _, _, n := protowire.ConsumeTag(bz) | ||
| bz, _ = protowire.ConsumeBytes(bz[n:]) | ||
|
|
||
| var ext v1alpha1.ModuleDescriptor | ||
| err = gogoproto.Unmarshal(bz, &ext) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return &ext, nil | ||
| } |
There was a problem hiding this comment.
Consider simplifying the descriptor handling in GetModuleDescriptor.
The function involves complex byte manipulations and conversions between different protobuf types. Consider simplifying these operations to improve maintainability and reduce the risk of errors.
- // Current complex byte manipulation
- _, _, n := protowire.ConsumeTag(bz)
- bz, _ = protowire.ConsumeBytes(bz[n:])
+ // Simplified approach (example, needs specific adjustment)
+ bz = simplifyByteHandling(bz)Committable suggestion was skipped due to low confidence.
|
This is the perfect branch for me to use for #20767. I'll test those changes on v0.50 and v0.47 |
| @@ -35,12 +38,18 @@ | |||
| fullName := gogoproto.MessageName(initializer.ConfigProtoMessage) | |||
|
|
|||
| if desc, err := gogoproto.HybridResolver.FindDescriptorByName(protoreflect.FullName(fullName)); err == nil { | |||
| modDesc := protov2.GetExtension(desc.Options(), appv1alpha1.E_Module).(*appv1alpha1.ModuleDescriptor) | |||
| dp := protodesc.ToDescriptorProto(desc.(protoreflect.MessageDescriptor)) | |||
Check warning
Code scanning / CodeQL
Useless assignment to local variable Warning
| @@ -35,12 +38,18 @@ func ModulesByModuleTypeName() (map[string]*ModuleInitializer, error) { | |||
| fullName := gogoproto.MessageName(initializer.ConfigProtoMessage) | |||
|
|
|||
| if desc, err := gogoproto.HybridResolver.FindDescriptorByName(protoreflect.FullName(fullName)); err == nil { | |||
| modDesc := protov2.GetExtension(desc.Options(), appv1alpha1.E_Module).(*appv1alpha1.ModuleDescriptor) | |||
| dp := protodesc.ToDescriptorProto(desc.(protoreflect.MessageDescriptor)) | |||
There was a problem hiding this comment.
compile err here, unused var
There was a problem hiding this comment.
Sorry I will clean it up. Was debugging and forgot to delete
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (2)
- depinject/internal/appconfig/registry.go (3 hunks)
- runtime/v2/go.mod (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- depinject/internal/appconfig/registry.go
- runtime/v2/go.mod
Description
This PR removes the dependency on the
apimodule indepinjectby generating the needed types indepinject/appconfigusing gogo proto and doing dynamic conversions using protoreflect as necessary.The modification of
Composeto take simplygogoproto.Messageshould be non-breaking for users (unless someone is using it as a function closure value which is unlikely).Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
Improvements
Documentation